@microsoft-github-policy-service agree company="Google"

@jrieken, would you be willing to take a look at this?

@trevorade Thanks for reaching out and starting this. This is a large PR and something we need to coordinate across the team. Usually, we have reservations against PRs that touch very many code owner areas but this seems mostly mechanical and exception-worthy.

I will circle this with the team and see how folks respond. Generally, I am positive about this change and it seems this must happen anyways sooner or later.

To truly enable useDefineForClassFields, more changes will more than likely need to be made

Do you have a feel for what that would be?

@mjbvz You initial concern in #186726 (comment) seems to have fixed, right? Any other insights this that you have?

@trevorade Thanks for reaching out and starting this. This is a large PR and something we need to coordinate across the team. Usually, we have reservations against PRs that touch very many code owner areas but this seems mostly mechanical and exception-worthy.

I will circle this with the team and see how folks respond. Generally, I am positive about this change and it seems this must happen anyways sooner or later.

Thx. This is exactly what I expected. That seeing such a large PR from a new contributor would raise some eyebrows and require some discussion.

Once you guys are fully comfortable with this PR, I will re-sync, again fix all new errors, and move forward with submit.

It would really be best if the codebase had some method to prevent backsliding but I'm unaware of any short of enabling useDefineForClassFields.

To truly enable useDefineForClassFields, more changes will more than likely need to be made

Do you have a feel for what that would be?

When you actually enable the flag, the code emitted changes quite a bit more. You start using true class fields rather than object properties throughout. While testing with this flag enabled, I wasn't able to actually run VSCode due to some errors. I forget exactly what. Easiest way to test that out would be to patch this PR and turn on the flag to experiment.

Anyways, my point is that I would expect that someone would want to do rigorous testing once enabling this flag. This PR is safer as it only fixes all the compiler errors without actually making the change.

@mjbvz You initial concern in #186726 (comment) seems to have fixed, right? Any other insights this that you have?

Matt was correct that there were additional runtime errors to consider. In the case of moving only the initializers that reference parameter properties, there's still the issue of other non-constructor initializers that access other fields initialized in the constructor from a function. This can be safe if the function is evaluated after the constructor or unsafe if evaluated immediately (i.e. via waitForState or recomputeInitiallyAndOnChange). I found a few instances of this in relation to this PR which I fixed here (from observing errors and then from analyzing all instances of waitForState or recomputeInitiallyAndOnChange for safety (i.e. usage in a field initializer versus inside a method).

I can only be as confident as your tests though. I would hope that someone in the VSCode team could patch this PR and test things out a bit to check for anything that may not be as well covered in tests.

Beyond what I fixed, it's possible there are other code execution order changes that will be introduced via enabling useDefineForClassFields that need to be fixed.

but I'm unaware of any short of enabling useDefineForClassFields.

We can prevent erosion by adding another tsconfig which enables useDefineForClassFields and do a separate check with that. Will slow down CI by a tiny bit, for the time being, but something that we have done successfully in the past